backport: bitcoin#23139, #24993, #25070, #25472, #25511, #25549, #25565, #25581, #25592, #25615 #6883
Conversation
… a file 027aab6 test, contrib, refactor: use `with` when opening a file (brunoerg) Pull request description: When manipulating a file in Python without using `with()`, you have to close the file manually, so this PR does it in `get_block_hashes` (`contrib/linearize/linearize-data.py`). Edit: this PR does it for all occurances that previously weren't using `with`. ACKs for top commit: laanwj: Code review ACK 027aab6 Tree-SHA512: 879400968e0013e8678ec16f1fe5d0963a73c1e0d442ca34802d885214f0783d2e9a9b500fc6be7c3b93560a367b6a3d685eee24d2f9ce53fddf064ea6feecf8
ded915e contrib: fix dirname on `verify-commits` (brunoerg) Pull request description: Fixes: https://github.com/bitcoin/bitcoin/runs/6309423255 ACKs for top commit: fanquake: ACK ded915e Tree-SHA512: fbc46e907ec6151aca76360b471f0f34b9fc7d7eb054616df61feaf392bc4710dc26a965adb432e91e18498d446787c388c7989d07e4858d0fbf6bf28074b24c
…t` should throw an error d22bd54 test: passing a non-positive integer value to `-peertimeout` should throw an error (brunoerg) Pull request description: This PR adds test coverage for bitcoin#25506, since bitcoin#25505 gets closed. ACKs for top commit: kristapsk: ACK d22bd54 w0xlt: ACK bitcoin@d22bd54 1440000bytes: ACK bitcoin@d22bd54 Tree-SHA512: 89c8a097606cb52569d816cc2227baac832df70e381d07c4a12aeb024c500d334c8102218fc6519eebb3819159d8308119d7253d9192a6bebe13b8e738b286b9
…ctions d3e9a1c doc: update for NetBSD 9.2, add GUI Build Instructions (Jarol Rodriguez) Pull request description: **For reviewer:** as I suppose few have a NetBSD system available, I wrote a [guide](https://gist.github.com/jarolrod/385dc063bb02c90aea0cbe8a147fc418#file-netbsd-vm-setup-guide-md) to setup a VM for testing purposes. This attempts to update the NetBSD docs so one can successfully build on the latest release. It also adds instructions to build the GUI. Additionally, it includes a note and an example on how one could update the gcc version bundled with NetBSD 9.2 and prior to be able to actually compile. This note can be updated with the release of NetBSD 10, as it will package an acceptable gcc version. Master: [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-netbsd.md) PR: [render](https://github.com/bitcoin/bitcoin/blob/d3e9a1c71bef1d730b5820f85e9758af54267ac3/doc/build-netbsd.md) Related to bitcoin#20610, but reworked. ACKs for top commit: aureleoules: ACK d3e9a1c. fanquake: ACK d3e9a1c Tree-SHA512: fc3c12689cee886f26782c1d57f3b794ceaedc965a571dd06cfc4a57f90393842ad2124e6dba55a12ac9de9bf63d8e3eb4aa541768f2aa8603248175ce7d1c08
…s_of_big_transactions` helper 6cbe65c test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper (Sebastian Falbesoner) Pull request description: Recently merged PR bitcoin#25522 (commit 2222842) enabled specifying an absolute fee for MiniWallet's `create_self_transfer` method. We can use that in the `create_lots_of_big_transactions` helper to avoid deducting the fee manually (with prior conversion from BTC to Satoshis). This helper is used (directly or indirectly) in the tests `feature_maxuploadtarget.py`, `mempool_limit.py`, `mining_prioritisetransaction.py`. ACKs for top commit: MarcoFalke: cr ACK 6cbe65c Tree-SHA512: 63d66939ae36722a2dc787cbd8f1f995de6232139c2169a3d25525f43c7aaacf646d86b4095a8078f26db18e916778c8097acb19ef17ab0f58382b8bb718d60b
dc02edc doc: update the URLs to thread functions in developer-notes (Vasil Dimov) c5cc3f1 doc: list the I2P accept thread in developer-notes (Vasil Dimov) Pull request description: Document `i2paccept` in `doc/developer-notes.md` and fix broken URLs to doxygen.bitcoincore.org. ACKs for top commit: theStack: re-ACK dc02edc Tree-SHA512: 7d396885dd2e8fda2b050aaa25a82b4217ced6a5aa3478339fb892d5392d2b8b6b5997f8bb9acaab7867c0c5bf58bd0b720ef36b335b1e7eb617b8fc205915b0
…d for stop_nodes()
a9790ba [test] persist prioritisation of transactions not in mempool (glozow) Pull request description: We persist tx prioritisation/fee deltas in mempool.dat (see `DumpMempool`). It seems we have test coverage for persistence of modified fees of mempool entries (see `vinfo` loop), but not for the prioritisation of transactions not in mempool (see `mapDeltas`). Relevant: bitcoin#25487 (comment) ACKs for top commit: darosior: utACK a9790ba w0xlt: ACK bitcoin@a9790ba Tree-SHA512: 3f2769a917041f12414584f69b2239eef57586f9975869e826f356633fcaf893fde15500619b302e7663de14f3661c6cba22c7524cab5286e715e2c105726521
630c171 refactor: Drop no longer needed `util/designator.h` (Hennadii Stepanov) 88ec5d4 build: Increase MS Visual Studio minimum version (Hennadii Stepanov) 555f9dd rpc, refactor: Add `decodepsbt_outputs` (Hennadii Stepanov) 0c432cb rpc, refactor: Add `decodepsbt_inputs` (Hennadii Stepanov) 01d95a3 rpc, refactor: Add `getblock_prevout` (Hennadii Stepanov) Pull request description: Visual Studio 2022 with `/std:c++20` supports [designated initializers](bitcoin#24531). ACKs for top commit: sipsorcery: reACK 630c171. Tree-SHA512: 5b8933940dd69061c6b077512168bebb6fea05d429b63ffbab191950798b4c825e8484b1a651db0ae13f97eae481097d3c16395659c0f3b9f847af2aaf44b65d
743a84a fix gettxout help text (Marnix) Pull request description: replaces bitcoin#25578 Add help text to asm & hex (like everywhere else). I've also changed two `RPCResult::Type::STR` to `RPCResult::Type::STR_HEX` Top commit has no ACKs. Tree-SHA512: 4109d6abddf71b24899f3252545248bb0c7cc366eb994d30927eb300d0b939a14b8140bac4a4c2bd45098a406666dbe1feb10da8dec923777bb8ed26784dfd54
…onString(), add coverage 66f6efc rpc: improve TransactionDescriptionString() "generated" help (Jon Atack) 296cfa3 test: add listtransactions/listsinceblock "trusted" coverage (Jon Atack) d95913f rpc: fix "trusted" description in TransactionDescriptionString (Jon Atack) Pull request description: The RPC gettransaction, listtransactions, and listsinceblock helps returned by `TransactionDescriptionString()` inform the user that the `trusted` boolean field is only present if the transaction is trusted and safe to spend from. The field is in fact returned by `WalletTxToJSON()` when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false. This patch fixes the help, adds test coverage, and touches up the help for the neighboring `generate` field. ACKs for top commit: rajarshimaitra: tACK bitcoin@66f6efc theStack: Tested ACK 66f6efc Tree-SHA512: 4c2127765b82780e07bbdbf519d27163d414d9f15598e01e02210f210e6009be344c84951d7274e747b1386991d4c3b082cd25aebe885fb8cf0b92d57178f68e
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes are a mix of refactors, documentation updates, RPC result schema reorganizations, and test adjustments:
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/rpc/transactions.cpp (1)
422-431: Doc key mismatch: use instantlock_internal (underscore), not instantlock-internal (hyphen).Current help documents "instantlock-internal", but code returns "instantlock_internal" (see WalletTxToJSON and other RPCs). Align the help key.
Apply this diff:
- {RPCResult::Type::BOOL, "instantlock-internal", "Current internal transaction lock state. Available for 'send' and 'receive' category of transactions"}, + {RPCResult::Type::BOOL, "instantlock_internal", "Current internal transaction lock state. Available for 'send' and 'receive' category of transactions"},
🧹 Nitpick comments (1)
test/functional/p2p_timeouts.py (1)
104-108: Consider clarifying the stop_nodes call.The test correctly validates that
-peertimeout=0raises an initialization error. However, the callself.stop_nodes(0)on line 104 is potentially confusing because it looks like it's stopping node index 0, when it's actually passingwait=0to stop all nodes.For clarity, consider using:
self.stop_nodes()(relies on default wait=0), orself.stop_nodes(wait=0)(explicitly names the parameter)Apply this diff for improved clarity:
- self.stop_nodes(0) + self.stop_nodes()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
contrib/devtools/copyright_header.py(1 hunks)contrib/linearize/linearize-data.py(2 hunks)contrib/linearize/linearize-hashes.py(1 hunks)contrib/verify-commits/verify-commits.py(1 hunks)doc/build-netbsd.md(1 hunks)doc/developer-notes.md(4 hunks)src/rpc/blockchain.cpp(3 hunks)src/rpc/rawtransaction.cpp(2 hunks)src/wallet/rpc/transactions.cpp(1 hunks)test/functional/feature_config_args.py(1 hunks)test/functional/feature_versionbits_warning.py(1 hunks)test/functional/mempool_persist.py(4 hunks)test/functional/p2p_timeouts.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/test_framework/util.py(1 hunks)test/functional/wallet_import_rescan.py(1 hunks)test/functional/wallet_listsinceblock.py(2 hunks)test/functional/wallet_listtransactions.py(1 hunks)test/util/test_runner.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/verify-commits/verify-commits.pycontrib/devtools/copyright_header.pycontrib/linearize/linearize-data.pycontrib/linearize/linearize-hashes.py
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/test_framework.pytest/functional/feature_config_args.pytest/functional/feature_versionbits_warning.pytest/functional/test_framework/util.pytest/functional/wallet_listtransactions.pytest/functional/wallet_listsinceblock.pytest/functional/p2p_timeouts.pytest/functional/wallet_import_rescan.pytest/functional/mempool_persist.py
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/developer-notes.mddoc/build-netbsd.md
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/rawtransaction.cppsrc/rpc/blockchain.cppsrc/wallet/rpc/transactions.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
🔇 Additional comments (19)
test/functional/wallet_listtransactions.py (1)
30-30: LGTM! Trusted field assertions added correctly.The test now verifies that:
- Sent transactions have
trusted: True(line 30)- Received unconfirmed transactions have
trusted: False(line 33)This aligns with the expected wallet behavior for transaction trust semantics.
Also applies to: 33-33
test/functional/test_framework/util.py (1)
620-621: LGTM! Fee handling simplified.The refactor passes the fee directly to
create_self_transfervia thefeeparameter instead of usingfee_rate=0and manually deducting fees. This simplifies the code and removes the in-function fee calculation logic.contrib/linearize/linearize-data.py (2)
37-42: LGTM! Context manager improves resource handling.The file reading is now wrapped in a
withstatement, ensuring the file is properly closed after reading. The logic remains unchanged.
252-263: LGTM! Context manager improves resource handling.The config file reading is now wrapped in a
withstatement, ensuring proper file closure. The parsing logic is unchanged.test/functional/test_framework/test_framework.py (1)
684-688: LGTM! Simplified API by removing expected_stderr parameter.The
stop_nodesmethod signature now only accepts thewaitparameter, removingexpected_stderrvalidation from batch node stops. Individual node stops viastop_node(singular) can still validate stderr when needed.test/util/test_runner.py (4)
25-26: LGTM! Context manager ensures proper file closure.Config file reading now uses a
withstatement, preventing resource leaks.
47-48: LGTM! Context manager ensures proper file closure.Test input file reading now uses a
withstatement for automatic cleanup.
85-86: LGTM! Context manager ensures proper file closure.Input data file reading now uses a
withstatement for proper resource management.
97-98: LGTM! Context manager ensures proper file closure.Expected output file reading now uses a
withstatement to ensure the file is closed after reading.test/functional/mempool_persist.py (3)
106-109: LGTM! Tests prioritisation persistence for unsubmitted transactions.The test creates and prioritises a transaction (with fee_delta=9999) before submitting it, verifying that prioritisation persists across node restarts even when the transaction hasn't been broadcast yet.
131-133: LGTM! Verifies prioritisation after restart.After restart, the test submits the previously prioritised transaction and confirms that its modified fee correctly reflects the 9999 satoshi fee delta applied before the restart.
120-120: LGTM! Expected transaction counts updated correctly.The counts are updated to reflect the additional prioritised transaction:
- Line 120: 6 transactions (5 original + 1 unbroadcast)
- Lines 152, 162, 167: 7 transactions (5 original + 1 unbroadcast + 1 prioritised)
Also applies to: 152-152, 162-162, 167-167
test/functional/wallet_listsinceblock.py (2)
48-54: LGTM! Adds synchronization and trusted field verification.The test now synchronizes nodes after sending the transaction and verifies that the unconfirmed received transaction has
trusted: False. This ensures proper transaction propagation before assertions.
67-68: LGTM! Verifies trusted field absence after confirmation.After block generation, the test confirms that:
- The transaction list contains exactly one entry
- The confirmed transaction does not include the
trustedfieldThis correctly validates the behavior where trust is only relevant for unconfirmed transactions.
src/rpc/rawtransaction.cpp (2)
1020-1086: Good extraction of decodepsbt inputs schema.Matches actual fields produced in decodepsbt. No issues.
1165-1167: LGTM: Reusing the new descriptors in decodepsbt.Keeps the schema DRY and consistent.
src/rpc/blockchain.cpp (3)
889-911: LGTM: getblock_vin descriptor is complete and consistent.Fields and optionality align with verbosity 3 behavior and undo availability.
971-974: LGTM: Properly plugs getblock_vin into verbosity=3 tx schema.Extends verbosity 2 with prevout details as intended.
1269-1272: LGTM: Clarified asm/hex descriptions in gettxout.Improves help text without altering behavior.
| ./configure | ||
| ... | ||
| CC="/usr/pkg/gcc9/bin/gcc" \ | ||
| CXX="/usr/pkg/gcc9/bin/g++" \ | ||
| ... |
There was a problem hiding this comment.
Fix the multiline ./configure example.
As written, the first line lacks a trailing \, so copy/pasting runs ./configure immediately and the remaining lines execute as separate commands (CC=…, CXX=…) and fail. Please fold the environment variables into the same multiline command, e.g.:
-./configure
- ...
- CC="/usr/pkg/gcc9/bin/gcc" \
- CXX="/usr/pkg/gcc9/bin/g++" \
- ...
+./configure \
+ ...
+ CC="/usr/pkg/gcc9/bin/gcc" \
+ CXX="/usr/pkg/gcc9/bin/g++" \
+ ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ./configure | |
| ... | |
| CC="/usr/pkg/gcc9/bin/gcc" \ | |
| CXX="/usr/pkg/gcc9/bin/g++" \ | |
| ... | |
| ./configure \ | |
| ... | |
| CC="/usr/pkg/gcc9/bin/gcc" \ | |
| CXX="/usr/pkg/gcc9/bin/g++" \ | |
| ... |
🤖 Prompt for AI Agents
doc/build-netbsd.md around lines 30 to 34: the multiline example splits into
separate commands because the first line "./configure" lacks a trailing
backslash; change the snippet so the environment variables and ./configure are
part of the same multiline command (either put the environment assignments on
preceding lines each ending with "\" and then a final "./configure" line, or add
a trailing "\" to the "./configure" line and place the CC/CXX=... lines
following it) so copying runs a single combined command.
There was a problem hiding this comment.
it's not working example, just mockup. Not expected to work after copy-paste. Also that's backported from bitcoin
There was a problem hiding this comment.
@knst, understood! Thank you for clarifying that this is a mockup example from the Bitcoin backport and not intended to be copy-pasted as-is. I'll note this and won't press the issue further.
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
| const RPCResult decodepsbt_outputs{ | ||
| RPCResult::Type::ARR, "outputs", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::OBJ, "redeem_script", /*optional=*/true, "", | ||
| { | ||
| {RPCResult::Type::STR, "asm", "The asm"}, | ||
| {RPCResult::Type::STR_HEX, "hex", "The hex"}, | ||
| {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"}, | ||
| }}, | ||
| {RPCResult::Type::ARR, "bip32_derivs", /*optional=*/true, "", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR, "pubkey", "The public key this path corresponds to"}, | ||
| {RPCResult::Type::STR, "master_fingerprint", "The fingerprint of the master key"}, | ||
| {RPCResult::Type::STR, "path", "The path"}, | ||
| }}, | ||
| }}, | ||
| {RPCResult::Type::OBJ_DYN, "unknown", /*optional=*/true, "The unknown global fields", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "key", "(key-value pair) An unknown key-value pair"}, | ||
| }}, | ||
| {RPCResult::Type::ARR, "proprietary", /*optional=*/true, "The output proprietary map", | ||
| { | ||
| {RPCResult::Type::OBJ, "", "", | ||
| { | ||
| {RPCResult::Type::STR_HEX, "identifier", "The hex string for the proprietary identifier"}, | ||
| {RPCResult::Type::NUM, "subtype", "The number for the subtype"}, | ||
| {RPCResult::Type::STR_HEX, "key", "The hex for the key"}, | ||
| {RPCResult::Type::STR_HEX, "value", "The hex for the value"}, | ||
| }}, | ||
| }}, | ||
| }}, | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Fix wording: outputs “unknown” are output fields, not global.
Minor description nit; prevents confusion with PSBT global unknowns.
Apply this diff:
- {RPCResult::Type::OBJ_DYN, "unknown", /*optional=*/true, "The unknown global fields",
+ {RPCResult::Type::OBJ_DYN, "unknown", /*optional=*/true, "The unknown output fields",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RPCResult decodepsbt_outputs{ | |
| RPCResult::Type::ARR, "outputs", "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::OBJ, "redeem_script", /*optional=*/true, "", | |
| { | |
| {RPCResult::Type::STR, "asm", "The asm"}, | |
| {RPCResult::Type::STR_HEX, "hex", "The hex"}, | |
| {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"}, | |
| }}, | |
| {RPCResult::Type::ARR, "bip32_derivs", /*optional=*/true, "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::STR, "pubkey", "The public key this path corresponds to"}, | |
| {RPCResult::Type::STR, "master_fingerprint", "The fingerprint of the master key"}, | |
| {RPCResult::Type::STR, "path", "The path"}, | |
| }}, | |
| }}, | |
| {RPCResult::Type::OBJ_DYN, "unknown", /*optional=*/true, "The unknown global fields", | |
| { | |
| {RPCResult::Type::STR_HEX, "key", "(key-value pair) An unknown key-value pair"}, | |
| }}, | |
| {RPCResult::Type::ARR, "proprietary", /*optional=*/true, "The output proprietary map", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::STR_HEX, "identifier", "The hex string for the proprietary identifier"}, | |
| {RPCResult::Type::NUM, "subtype", "The number for the subtype"}, | |
| {RPCResult::Type::STR_HEX, "key", "The hex for the key"}, | |
| {RPCResult::Type::STR_HEX, "value", "The hex for the value"}, | |
| }}, | |
| }}, | |
| }}, | |
| } | |
| }; | |
| const RPCResult decodepsbt_outputs{ | |
| RPCResult::Type::ARR, "outputs", "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::OBJ, "redeem_script", /*optional=*/true, "", | |
| { | |
| {RPCResult::Type::STR, "asm", "The asm"}, | |
| {RPCResult::Type::STR_HEX, "hex", "The hex"}, | |
| {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"}, | |
| }}, | |
| {RPCResult::Type::ARR, "bip32_derivs", /*optional=*/true, "", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::STR, "pubkey", "The public key this path corresponds to"}, | |
| {RPCResult::Type::STR, "master_fingerprint", "The fingerprint of the master key"}, | |
| {RPCResult::Type::STR, "path", "The path"}, | |
| }}, | |
| }}, | |
| {RPCResult::Type::OBJ_DYN, "unknown", /*optional=*/true, "The unknown output fields", | |
| { | |
| {RPCResult::Type::STR_HEX, "key", "(key-value pair) An unknown key-value pair"}, | |
| }}, | |
| {RPCResult::Type::ARR, "proprietary", /*optional=*/true, "The output proprietary map", | |
| { | |
| {RPCResult::Type::OBJ, "", "", | |
| { | |
| {RPCResult::Type::STR_HEX, "identifier", "The hex string for the proprietary identifier"}, | |
| {RPCResult::Type::NUM, "subtype", "The number for the subtype"}, | |
| {RPCResult::Type::STR_HEX, "key", "The hex for the key"}, | |
| {RPCResult::Type::STR_HEX, "value", "The hex for the value"}, | |
| }}, | |
| }}, | |
| }}, | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/rpc/rawtransaction.cpp around lines 1088 to 1125, the RPCResult
description for the "unknown" OBJ_DYN entry mistakenly calls them "The unknown
global fields" which is incorrect for outputs; change that description to
indicate these are unknown output fields (e.g. "The unknown output fields" or
similar) so it correctly reflects that these unknowns belong to the output scope
rather than PSBT globals.
There was a problem hiding this comment.
@coderabbitai consider "moved" code, especially in backports, as "out-of-scope". Please validate instead if code is moved correctly, no fields are added, no fields are moved, no unexpected changes.
Whitespaces are expected to be changed in this particular case.
There was a problem hiding this comment.
@knst, thank you for the clarification. You're absolutely right—for backport PRs, I should focus on validating that the code is moved correctly (fields preserved, no unexpected modifications) rather than flagging issues that may already exist in the upstream Bitcoin Core code.
I'll withdraw my suggestion about the description text. If that wording was already present in the upstream code being backported, then maintaining it as-is ensures fidelity to the original implementation.
Going forward, I'll apply this approach to backport reviews: verify the move is accurate while treating pre-existing content as out-of-scope.
✏️ Learnings added
Learnt from: knst
PR: dashpay/dash#6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.326Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
5 participants
What was done?
Regular backports from Bitcoin Core v24
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
N/A
Checklist: